Skip to content

fix(slack): fall back to attachment text in thread context for bot messages - #33493

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/slack-thread-context-attachments
Closed

fix(slack): fall back to attachment text in thread context for bot messages#33493
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/slack-thread-context-attachments

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Bot messages from monitoring tools (Datadog, PagerDuty, Grafana, etc.) often use the legacy Slack "attachments" format instead of top-level text. When Hermes is @mentioned in a thread whose parent was posted by such a bot, the parent message is silently dropped from thread context because its top-level text field is empty — the actual content lives in attachments[].text or attachments[].fallback.

This fix adds a fallback: when msg_text is empty, the code now checks attachments[].text then attachments[].fallback before skipping the message.

Related Issue

Fixes #33469

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • See commit messages for detailed changes

How to Test

  1. Run pytest tests/ -q — all tests should pass
  2. Verify the specific scenario described above is resolved

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture and workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

Code Intelligence

  • Analyzed: gateway/platforms/slack.py _fetch_thread_context (callers: 1 — message handler)
  • Blast radius: LOW — single function, additive fallback, no behavior change for existing non-empty text messages
  • Related patterns: _fetch_thread_parent_text uses the same cache; no changes needed there since it has its own fallback logic

Fixes #33469

@liuhao1024
fix(slack): fall back to attachment text in thread context for bot me…
c7d0ccc 
…ssages

Bot messages from monitoring tools (Datadog, PagerDuty, Grafana) often
have empty top-level text with all content in attachments[].text or
attachments[].fallback. _fetch_thread_context silently dropped these,
leaving Hermes unaware of the thread origin.

Fall back to attachments[].text then attachments[].fallback when the
top-level text is empty, so alert context is preserved in thread replies.

Fixes NousResearch#33469
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists platform/slack Slack app adapter comp/gateway Gateway runner, session dispatch, delivery labels May 27, 2026
@alt-glitch

alt-glitch commented May 27, 2026

Copy link
Copy Markdown
Contributor

Overlaps with open #30765 (include alert attachments in thread context) and addresses #33469 (bot messages with attachments silently skipped). Merged #16200 previously fixed bot-posted thread parent context but did not cover legacy attachment-only messages.

@SebasSotoA

SebasSotoA commented May 29, 2026

Copy link
Copy Markdown

Please approve this, my team needs this change ASAP

@alt-glitch alt-glitch mentioned this pull request Jul 9, 2026
Closed
@teknium1

teknium1 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the focused regression coverage. The reported defect is still present on current main: plugins/platforms/slack/adapter.py:3788-3790 reads only msg["text"] and drops attachment-only thread messages.

Problems

  • The production diff changes gateway/platforms/slack.py, but current main relocated the active adapter to plugins/platforms/slack/adapter.py in 5600105478ffde29d7566b45421b100eaa29c4ef. The submitted source change therefore does not affect the live Slack adapter.
  • The active cold parent lookup has the same gap at plugins/platforms/slack/adapter.py:3891; it reads only parent["text"]. Porting only _fetch_thread_context would still leave attachment-only parents absent from reply_to_text when the cache is cold.

Suggested changes

  • Port the fallback to the active plugin adapter and reuse it in both _fetch_thread_context and _fetch_thread_parent_text.
  • Add a cold parent-lookup regression case alongside the thread-context cases.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@alt-glitch alt-glitch added comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation and removed comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists labels Jul 13, 2026
@alt-glitch

alt-glitch commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Heads up: this PR edits gateway/platforms/slack.py, but the Slack adapter has since migrated to plugins/platforms/slack/adapter.py on main — it will need a rebase onto the relocated path before it can merge. Competing/superset PR #61261 already targets the relocated adapter and additionally covers Block Kit blocks + rich attachment extraction + parent-text; #52219 is a sibling on the distinct inbound-message-text code path. Maintainer to pick the canonical fix.

@alt-glitch alt-glitch added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages and removed sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 13, 2026
@teknium1 teknium1 mentioned this pull request Jul 22, 2026
Merged
@teknium1

teknium1 commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Closing as superseded by #69316 (merged): targeted the pre-plugin-migration gateway/platforms/slack.py which no longer exists; functionality covered on the plugin path.

Thanks for digging into this — the consolidated fix stands on the cluster's collective analysis, and your work is credited in #69316's summary.

@teknium1 teknium1 closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: [Slack] Thread parent messages from bots using attachments (e.g. Datadog) are silently skipped in _fetch_thread_context

4 participants

@liuhao1024 @alt-glitch @SebasSotoA @teknium1